Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OPSIM-1063: Ensure MAF imports without data and that all stackers run #349

Merged
merged 11 commits into from
Aug 24, 2023
Merged

OPSIM-1063: Ensure MAF imports without data and that all stackers run #349

merged 11 commits into from
Aug 24, 2023

Conversation

rhiannonlynne
Copy link
Member

If load_inst_zeropoints is in init method, then the actual throughput files are attempted to be read as soon as the stacker is instantiated (as in the batches or sometimes in metrics init methods).
Moving the call to load_inst_zeropoints to the run method lets rubin_sim import even without data being available, instead of failing to import in that case.

yoachim
yoachim approved these changes Aug 10, 2023
View reviewed changes
rubin_sim/maf/stackers/general_stackers.py Outdated
self.zeropoints = zeropoints
self.km = km

def _run(self, sim_data, cols_present=False):
if zeropoints is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be self.zeropoints. That implies there's no unit test coverage of the _run method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun - I added a unit test to run all of the stacker 'run' methods and I'm finding a few others which don't work.
Scope creep, but should be able to fix these.

@rhiannonlynne rhiannonlynne changed the title Move load_inst_zeropoints to run OPSIM-1036: Move load_inst_zeropoints to run Aug 23, 2023
@rhiannonlynne rhiannonlynne changed the title OPSIM-1036: Move load_inst_zeropoints to run OPSIM-1063: Move load_inst_zeropoints to run Aug 23, 2023
@rhiannonlynne
Copy link
Member Author

The scope of this PR expanded once I added a unit test to make sure that the SaturationStacker ran.
I set up the unit test to run all of the possible stackers (i.e. not BaseStackers or moving object stackers, purely because I didn't have an easy data source for those .. but it would be good to add next). This meant fixing up a couple of other stackers which didn't work with a full datasource automatically or maybe worked a little less ideally than we would typically use them now.
I then tossed all of the dither stackers, as many of them required "fieldID" which is not a stacker we provide any longer.
This then meant removing dither stacker references from other locations (in the batches, primarily).

@rhiannonlynne rhiannonlynne changed the title OPSIM-1063: Move load_inst_zeropoints to run OPSIM-1063: Ensure MAF imports without data and that all stackers run Aug 24, 2023
yoachim
yoachim approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@yoachim yoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all looks reasonable.

@rhiannonlynne rhiannonlynne merged commit 0a5d21e into main Aug 24, 2023
3 of 4 checks passed
@rhiannonlynne rhiannonlynne deleted the tickets/OPSIM-1063 branch August 24, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoachim yoachim yoachim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rhiannonlynne @yoachim